Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add primary associated types to SignalProducerConvertible & SignalProducerProtocol #855

Merged

Conversation

braker1nine
Copy link
Contributor

@braker1nine braker1nine commented Oct 3, 2022

Adds primary associated types to SignalProducerConvertible and SignalProducerProtocol

Checklist

  • Updated CHANGELOG.md.

@NachoSoto
Copy link
Member

This breaks compilation with older compiler versions though.

@braker1nine
Copy link
Contributor Author

Ahhh, I was afraid of that. Let me see if I can handle that better 🤔

@braker1nine
Copy link
Contributor Author

Made a first pass at this. I'm not going to lie, I don't love it. Requires a bunch of repeated code...

Looks like the original proposal had the concept of a primary keyword, which would have made this much nicer. Unfortunately, from what I can tell that didn't make it into the final accepted version. 😞

@NachoSoto
Copy link
Member

I think this could work. We need to make sure CI tests run on Xcode 14 to test this too.

@braker1nine
Copy link
Contributor Author

Tweaked the test.yml to run on both macOS-12 and macOS-11. My GitHub Actions knowledge is a little limited so let me know if I did anything dumb.

@braker1nine
Copy link
Contributor Author

Just following up on this. @NachoSoto @mluisbrown is there anything you need me to do to help get this merged

@mluisbrown
Copy link
Contributor

@braker1nine we should try and have CI passing. There are issues with macCatalyst but the rest should pass ok.

@braker1nine
Copy link
Contributor Author

Oh sheesh. I missed that. Sorry @mluisbrown!

@braker1nine
Copy link
Contributor Author

I made a bit of a tweak to the testing structure on this one to try to increase the coverage of Swift versions

  • Testing on Xcode with both 13.4.1 and 14.1, so we hit both Swift 5.6 and 5.7
  • Testing Swift 5.2 (the minimum supported version for ReactiveSwift I think?), 5.6, and 5.7 on Linux SwiftPM. Also changed that one to use the setup-swift action instead of pulling it in via the swift version manager script

@NachoSoto
Copy link
Member

What do you think of opening a separate PR with the CI changes? We can merge that right away then I'll take a look at this one 🙏

@mluisbrown
Copy link
Contributor

What do you think of opening a separate PR with the CI changes? We can merge that right away then I'll take a look at this one 🙏

@NachoSoto we also need to address the fact that Mac Catalyst CI jobs are simply not working. IMO we should just disable the Mac Catalyst job until we have a better idea of what the issue is, as it's going to cause issues with all PRs.

What happens is that the job starts and then just hangs for ever until GitHub Actions decides to cancel it (hours later). If I run the exact same xcodebuild command locally it runs just fine, so there is no clue as to what the problem is.

The issue is affecting this PR and also this one and affected this one too. @p4checo tried using Xcode 14.1 in his PR to see if it made a difference and it didn't 🤷‍♀️

@braker1nine
Copy link
Contributor Author

braker1nine commented Nov 9, 2022

What do you think of opening a separate PR with the CI changes? We can merge that right away then I'll take a look at this one 🙏

Yeah I can absolutely do that.

@NachoSoto we also need to address the fact that Mac Catalyst CI jobs are simply not working. IMO we should just disable the Mac Catalyst job until we have a better idea of what the issue is, as it's going to cause issues with all PRs.

@mluisbrown Would it be worth disabling Catalyst in that same PR?

@mluisbrown
Copy link
Contributor

Would it be worth disabling Catalyst in that same PR?

@braker1nine yes, absolutely 👍

@braker1nine
Copy link
Contributor Author

Opened up #858. I didn't disable Catalyst in there, but I could add that

@RuiAAPeres RuiAAPeres merged commit ade1518 into ReactiveCocoa:master Nov 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants